-
Notifications
You must be signed in to change notification settings - Fork 465
Preserve JSX #7387
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Preserve JSX #7387
Conversation
Got an initial result here, was able to transform: module ReactDOM = {
@module("react/jsx-runtime")
external jsx: (string, JsxDOM.domProps) => Jsx.element = "jsx"
}
let _ = <div className="meh"></div> into // Generated by ReScript, PLEASE EDIT WITH CARE
'use strict';
let JsxRuntime = require("react/jsx-runtime");
let ReactDOM = {};
<div className={"meh"}></div>;
exports.ReactDOM = ReactDOM; There are still a lot of cases to cover, but it is a start. |
Made some more progress, was able to transform my entire project.
|
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/win32-x64
commit: |
This reverts commit 547dfca.
@cknitt you can test this by: npm i "https://pkg.pr.new/rescript-lang/rescript@f10452935ae8bc0325116323ee9b55f61c1827be"
"suffix": ".res.jsx",
"jsx": {
"version": 4
},
"bsc-flags": [
"-bs-jsx-preserve"
] (suffix not strictly required) |
@cristianoc, this is now functioning effectively. Could you please review this approach? If it seems reasonable, we can refine it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good.
Main question is if there's a way to avoid extending every Lprim
with a boolean, and the scope of changes can be restricted somehow.
let fields = ("key", key) :: fields in | ||
print_jsx cxt ~level f fnName tag fields | ||
| _ -> | ||
expression_desc cxt ~level f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this code branch ever executed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is, it means we missed something. Could we add some sort of assert in this case?
@cristianoc could you check e30cab3 and 773124f as well. Had to write some new to deal with prop spreading. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor style comments, plus the test does not seem to use preserve mode at the moment?
compiler/core/lam_compile.ml
Outdated
{ | ||
arity = Full; | ||
call_info = Call_ml; | ||
(* no clue if this is correct *) call_transformed_jsx = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use one of the existing default constants for this -- seems likely to be correct as these are generated applications, not coming from jsx ppx
compiler/core/lam_compile.ml
Outdated
{ | ||
arity = Full; | ||
call_info = Call_ml; | ||
(* no clue if this is correct *) call_transformed_jsx = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
(E.call | ||
~info: | ||
{ | ||
arity = Full; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about {...one_of_those_constants, transformed_jsx}
{ | ||
arity = Full; | ||
call_info = Call_na; | ||
call_transformed_jsx = transformed_jsx; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
~info: | ||
{ | ||
arity = Full; | ||
call_info = Call_na; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
~info: | ||
{ | ||
arity = Full; | ||
call_info = Call_na; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
arity = Full; | ||
call_info = Call_na; | ||
call_transformed_jsx = transformed_jsx; | ||
} | ||
(E.dot self name) args) | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
~info: | ||
{ | ||
arity = Full; | ||
call_info = Call_na; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
~info: | ||
{ | ||
arity = Full; | ||
call_info = Call_na; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to go!
Merge away after any final small cleanup.
@cristianoc, this doesn't work yet, but it highlights the idea. Could you point out where the information is not passed? It's a bit overwhelming for me to grasp.